-
Notifications
You must be signed in to change notification settings - Fork 512
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: Unify Client, Pool & Agent #620
Conversation
This comment has been minimized.
This comment has been minimized.
484d0fc
to
4260bdf
Compare
This needs more work but I'd like to have some initial feedback before spending more time. |
The Client interface basically becomes: interface Client implements EventEmitter {
dispatch(opts, handler): Boolean
destroy(err, callback)
'drain' (origin, clients)
'connect' (origin, clients)
'disconnect' (origin, clients, err)
// These are not really necessary for the required API
close(callback)
closed: Boolean
destroyed: Boolean
connected: Integer
size: Integer
running: Integer
pending: Integer,
} |
Huge +1 to this. |
+1 for me as well. I'd just wait for #603 (with my improved coverage which is coming up) to land so we can rely on tests. |
@jonnydgreen FYI, WIP. |
Looks awesome, thanks for keeping me in the loop :) |
491796b
to
68b44e6
Compare
e66d8b2
to
f915b49
Compare
86d1cd9
to
d88585d
Compare
+100 from me. I really like the new API! Go for it! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are benchmarks impacted by this change?
lib/agent.js
Outdated
.on('disconnect', this[kOnDisconnect]) | ||
.on('drain', this[kOnDrain]) | ||
|
||
this[kClients].set(opts.origin, new WeakRef(client)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don’t cleanup. It’s a minor mem leak. I personally think it’s fine but would like some more opinions.
Is this what is described in #631? It should only be a temporary leak from my understanding of it.
@jonnydgreen do you think this API would be ok for you to support in #587 |
@mcollina yep, definitely! Looking at the code, I don't see any major problems when it comes to adding mocking support. If anything, I think it will make everything a lot cleaner to implement and opens up a lot of additional functionality in the mocking support as I can use |
We have the benchmark CI 😄 . branch:
master:
Seems faster if anything. |
I will run more in depth benchmarks on the same EPYC machine the README results come from before release. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks great. really happy to see the core API refactor for easier client instantiations
What would entail in solving the memory leak? Can't we do the connect/disconnect solution that we used in main? |
I don’t like/trust that solution but I can look into it again for node 12. |
Not sure if feasible, but we could keep the |
Only on Node v12. It's definitely better than leaking. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
* refactor: Client.clients & Agent is a Client. Refs: nodejs#616 * fixup: rename clientt to dispatcher * fixup: keep promise chain * fixup * fixup * fixup
Client
,Pool
andAgent
under a single interface.Agent.get
(in favor of extra args to client connect/disconnect).Client/Pool.busy
in favor of return value ondispatch
. (I'm not 100% sure this is possible, but I think so).connect/disconnect
emit an array of entire connect/disconnect chain for introspection.'drain'
emit origin as first argument.Agent
a client with all the api methods (request, stream etc...) on the prototype and same events asClient
.So
Agent
,Pool
andClient
all implement the same interface and could be used interchangeably.Fixes:
#610
#616
#612